feat: CI - Add code rules checking action #3914

arng40 · 2025-11-18T13:00:04Z

This PR aim to create a new CI Target in order to ensure the use of the internal tool in geos and to promote the best coding practice.
First, this PR set up the CI check_code_rules and will ensure the use of : stdMap, stdUnorderedMap& stdVector
This PR will be merged after :

…EV/GEOS into feat/dudes/add-code-rules

MelReyCG

I think you showed that script works, but it seems that there are some critical issues in it.

MelReyCG · 2025-11-26T16:31:19Z

.github/workflows/ci_tests.yml

+          # - name: Ubuntu debug (20.04, gcc 10.5.0, open-mpi 4.0.3) - github codespaces
+          #   CMAKE_BUILD_TYPE: Debug
+          #   DOCKER_REPOSITORY: geosx/ubuntu20.04-gcc10
+          #   BUILD_SHARED_LIBS: ON
+          #   ENABLE_HYPRE: OFF
+          #   ENABLE_TRILINOS: ON
+          #   GEOS_ENABLE_BOUNDS_CHECK: ON
+          #   HOST_CONFIG: /spack-generated.cmake


you cannot leave CI modifs aimed for test and request review.

MelReyCG · 2025-11-26T16:32:03Z

.github/workflows/ci_tests.yml

+        # - name: Check code rules
+        #   BUILD_AND_TEST_ARGS: --test-code-rules


Why is it commented out?

MelReyCG · 2025-11-27T11:01:21Z

scripts/check_code_rules.sh

+FULL_FILE_PATTERN="${FILE_PREFIX}${FILE_PATTERNS[*]}"
+# Build the find command
+FIND_CMD="find"
+FIND_FILE_CMD="$FIND_CMD $FILE_PATH_PATERN"' \( -name "*.hpp" -o -name "*.cpp" \)'


FILE_PATH_PATTERN seems never initialized.

should you add -f?

Beware of template files, they should be included

GEOS/src/coreComponents/physicsSolvers/KernelDispatchTypeList.hpp.template

Lines 1 to 37 in 1e617be

/*

* ------------------------------------------------------------------------------------------------------------

* SPDX-License-Identifier: LGPL-2.1-only

*

* Copyright (c) 2018-2019 Lawrence Livermore National Security LLC

* Copyright (c) 2018-2019 The Board of Trustees of the Leland Stanford Junior University

* Copyright (c) 2018-2019 Total, S.A

* Copyright (c) 2019- GEOSX Contributors

* All right reserved

*

* See top level LICENSE, COPYRIGHT, CONTRIBUTORS, NOTICE, and ACKNOWLEDGEMENTS files for details.

* ------------------------------------------------------------------------------------------------------------

*/

/**

* @file @[email protected]

*/

#ifndef GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP

#define GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP

#include "common/TypeDispatch.hpp"

namespace geos

{

// TODO remove and find a proper solution to namespacing

using namespace constitutive;

using namespace finiteElement;

using @KERNEL_GROUP_NAME@DispatchTypeList = types::TypeList<

@typeCombinationList@

>;

} // namespace geos

#endif //GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP

MelReyCG · 2025-11-27T11:02:09Z

scripts/check_code_rules.sh

+FILES=$(eval "$FIND_FILE_CMD" 2>/dev/null);
+
+# Main loop
+for file in $FILES; do


How does it behave with spaces in folder / file names?

MelReyCG · 2025-11-27T11:05:55Z

scripts/check_code_rules.sh

+ARRAY_UMAP=()
+ARRAY_VECTOR=()
+
+FULL_FILE_PATTERN="${FILE_PREFIX}${FILE_PATTERNS[*]}"


I'm not sure to get that right, but does it result to the following?
src/coreComponents/codingUtilities/* common/* dataRepository/* ...

If yes, how can it work as the file prefix is not added to each file pattern? maybe it would be easier with a cd FILE_PREFIX (rename to VALIDATING_FOLDER?)

There was a missing part to this code..

MelReyCG · 2025-11-27T11:07:38Z

scripts/check_code_rules.sh

+  print_violation "$MAP_VIOLATIONS_FOUND" ARRAY_MAP "std::map"
+  print_violation "$UMAP_VIOLATIONS_FOUND" ARRAY_UMAP "std::unordered_map"
+  print_violation "$VECTOR_VIOLATIONS_FOUND" ARRAY_VECTOR "std::vector"


Add an array for them,

did you missed std::array?

array still not in GEOS

jafranc · 2025-11-28T13:05:19Z

src/coreComponents/schema/schema.xsd.other

 			<xsd:element name="SinglePhaseHybridFVM" type="SinglePhaseHybridFVMType" />
 			<xsd:element name="SinglePhasePoromechanics" type="SinglePhasePoromechanicsType" />
 			<xsd:element name="SinglePhasePoromechanicsConformingFractures" type="SinglePhasePoromechanicsConformingFracturesType" />
+			<xsd:element name="SinglePhasePoromechanicsConformingFracturesALM" type="SinglePhasePoromechanicsConformingFracturesALMType" />


why this there ? Branching oddity ?

jafranc · 2025-11-28T13:09:44Z

scripts/ci_build_and_test_in_container.sh

  --test-code-style
  --test-documentation
-  -h | --help
+    -h | --help


maybe reversible whitespace edition for PR's clarity sake

jafranc · 2025-11-28T13:21:57Z

.github/workflows/ci_tests.yml

    - are_submodules_in_sync
-    - check_code_style_and_documentation
-    - cpu_builds
+    - check_code_standards


If check_code_standards is just rename of check_code_style_and_documentation why ?
And why check_code_rules is not required here ?

Looking at CI checks, it is indeed then an advisory but not required enforcement ?

i'll revert that

jafranc · 2025-11-28T13:22:47Z

.github/workflows/ci_tests.yml

-          needs.check_code_style_and_documentation.result == 'success' &&
-          needs.cpu_builds.result == 'success' &&
-          needs.cuda_builds.result == 'success' && 
+          needs.check_code_standards.result == 'success' &&


was need.cpu_builds.results and its cuda counterpart removed while dev to speed up or is it permanent ??

it was to speed up

jafranc · 2025-11-28T13:27:59Z

scripts/check_code_rules.sh

+
+FULL_FILE_PATTERN="${FILE_PREFIX}${FILE_PATTERNS[*]}"
+# Build the find command
+FIND_CMD="find"


Since you control distribution and set only POSIX-compliant maybe it is useless to overcomplexify there

jafranc · 2025-11-28T13:31:08Z

scripts/check_code_rules.sh

+FULL_FILE_PATTERN="${FILE_PREFIX}${FILE_PATTERNS[*]}"
+# Build the find command
+FIND_CMD="find"
+FIND_FILE_CMD="$FIND_CMD $FILE_PATH_PATERN"' \( -name "*.hpp" -o -name "*.cpp" \)'


jafranc · 2025-11-28T13:31:59Z

scripts/check_code_rules.sh

+          "mesh/*"
+          "physicsSolvers/*"
+        )
+  EXCLUDE_PATTERNS=(


jafranc · 2025-11-28T13:36:58Z

scripts/check_code_rules.sh

+FULL_FILE_PATTERN="${FILE_PREFIX}${FILE_PATTERNS[*]}"
+# Build the find command
+FIND_CMD="find"
+FIND_FILE_CMD="$FIND_CMD $FILE_PATH_PATERN"' \( -name "*.hpp" -o -name "*.cpp" \)'


Beware of template files, they should be included

GEOS/src/coreComponents/physicsSolvers/KernelDispatchTypeList.hpp.template

Lines 1 to 37 in 1e617be

/*

* ------------------------------------------------------------------------------------------------------------

* SPDX-License-Identifier: LGPL-2.1-only

*

* Copyright (c) 2018-2019 Lawrence Livermore National Security LLC

* Copyright (c) 2018-2019 The Board of Trustees of the Leland Stanford Junior University

* Copyright (c) 2018-2019 Total, S.A

* Copyright (c) 2019- GEOSX Contributors

* All right reserved

*

* See top level LICENSE, COPYRIGHT, CONTRIBUTORS, NOTICE, and ACKNOWLEDGEMENTS files for details.

* ------------------------------------------------------------------------------------------------------------

*/

/**

* @file @[email protected]

*/

#ifndef GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP

#define GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP

#include "common/TypeDispatch.hpp"

namespace geos

{

// TODO remove and find a proper solution to namespacing

using namespace constitutive;

using namespace finiteElement;

using @KERNEL_GROUP_NAME@DispatchTypeList = types::TypeList<

@typeCombinationList@

>;

} // namespace geos

#endif //GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP

add-code-rules

6f8f1c0

arng40 requested review from paveltomin and rrsettgast as code owners November 18, 2025 13:00

arng40 self-assigned this Nov 18, 2025

arng40 added type: CI Concerns github workflows or generic CI DO NOT MERGE ! flag: no rebaseline Does not require rebaseline labels Nov 18, 2025

arng40 added 23 commits November 18, 2025 14:23

comment jobs not needed

243af07

try this

7754898

remove ctest testcoderule

29f0ed6

try to add some target

3d2782a

relaunch ci

38748b0

forget 1 target

21956b3

test code for detecting std::map

183262c

force to true

1a50cc9

log

4004d12

more log

42d6f5b

print pwd

6a87699

test with regex

3ae12c4

fix

06b6d8f

update find

9f65bda

update prefix file pattern

e519b29

move to script

09cea71

fix rule ^^

716ce69

test perm

a7f4a14

add write

fad5944

give perm

d3a6d9c

give perm

56eebfc

attempt perm

933dcc0

remove if statement

fc78670

arng40 and others added 8 commits November 20, 2025 11:45

Merge branch 'develop' into feat/dudes/add-code-rules

d10651d

fix declaration

ae7e57c

Merge branch 'feat/dudes/add-code-rules' of https://github.com/GEOS-D…

b30ef2a

…EV/GEOS into feat/dudes/add-code-rules

log

0dbd595

update echo

7f5aa44

refacto find cmd

84d15e9

remove exclude find

bc8e010

remove log + revert

74cad67

MelReyCG changed the title ~~feat: CI - Add code rules~~ feat: CI - Add code rules checking Nov 27, 2025

MelReyCG changed the title ~~feat: CI - Add code rules checking~~ feat: CI - Add code rules checking script Nov 27, 2025

MelReyCG changed the title ~~feat: CI - Add code rules checking script~~ feat: CI - Add code rules checking action Nov 27, 2025

MelReyCG requested changes Nov 27, 2025

View reviewed changes

jafranc requested changes Nov 28, 2025

View reviewed changes

update find command

44f564e

arng40 requested review from rasimHZ and tjb-ltk as code owners December 1, 2025 15:40

arng40 added 10 commits December 1, 2025 16:43

remove type f

46a0cf0

remove var from find

addf93f

reset find

cf5fea6

revert loop

3444008

update loop

72e8258

update code

10d7a2e

enforce code

2ebbc31

echo file

a1b5b2f

fix loop

92a66f3

revert some ci change + remove echo

e207a68

jafranc mentioned this pull request Dec 3, 2025

docs: Adding Code rules page #3915

Open

arng40 and others added 3 commits December 3, 2025 11:40

print all path + some refacto

d2b7019

remove debug log

f73cb6d

Merge branch 'develop' into feat/dudes/add-code-rules

c8d97a4

		# - name: Check code rules
		# BUILD_AND_TEST_ARGS: --test-code-rules

	/*
	* ------------------------------------------------------------------------------------------------------------
	* SPDX-License-Identifier: LGPL-2.1-only
	*
	* Copyright (c) 2018-2019 Lawrence Livermore National Security LLC
	* Copyright (c) 2018-2019 The Board of Trustees of the Leland Stanford Junior University
	* Copyright (c) 2018-2019 Total, S.A
	* Copyright (c) 2019- GEOSX Contributors
	* All right reserved
	*
	* See top level LICENSE, COPYRIGHT, CONTRIBUTORS, NOTICE, and ACKNOWLEDGEMENTS files for details.
	* ------------------------------------------------------------------------------------------------------------
	*/

	/**
	* @file @[email protected]
	*/

	#ifndef GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP
	#define GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP

	#include "common/TypeDispatch.hpp"

	namespace geos
	{

	// TODO remove and find a proper solution to namespacing
	using namespace constitutive;
	using namespace finiteElement;

	using @KERNEL_GROUP_NAME@DispatchTypeList = types::TypeList<
	@typeCombinationList@
	>;

	} // namespace geos

	#endif //GEOS_@KERNEL_GROUP_NAME@_DISPATCHTYPELIST_HPP

feat: CI - Add code rules checking action #3914

Are you sure you want to change the base?

feat: CI - Add code rules checking action #3914

Conversation

arng40 commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MelReyCG left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arng40 commented Nov 18, 2025 •

edited

Loading